Skip to content

More telemetry events#133

Open
rkallos wants to merge 11 commits intolpgauth:masterfrom
rkallos:more-telemetry-events
Open

More telemetry events#133
rkallos wants to merge 11 commits intolpgauth:masterfrom
rkallos:more-telemetry-events

Conversation

@rkallos
Copy link
Copy Markdown
Contributor

@rkallos rkallos commented Aug 2, 2023

A couple of changes for your consideration:

  1. Storing Request values (understood by implementors of shackle_client) in shackle_queue, and passing them to shackle_telemetry:reply/4 and shackle_telemetry:timeout/2. I added this to get the HTTP status code and URL path from buoy.
  2. Adding shackle_telemetry:connection_error/2 and shackle_telemetry:queued_time/2.
  3. Using erlang:monotonic_time/{0,1} to measure elapsed time, instead of os:timestamp/0 and timer:now_diff/2. This is specifically called out in the Time and Time Correction in Erlang, though it mentions erlang:timestamp/0. It does, however, recommend using erlang:monotonic_time/0 and subtraction to measure elapsed time between events.

I got rid of a couple of ?WARN invocations because they can be pretty spammy. I'm not sure what the best approach is to handling these log messages, or whether they can be adequately replaced with metrics. I'd be happy to add them back if you want.

@rkallos rkallos force-pushed the more-telemetry-events branch from 3da0467 to e3d8a34 Compare August 14, 2023 18:02
@lpgauth
Copy link
Copy Markdown
Owner

lpgauth commented Aug 15, 2023

I think it might be a good idea to also include a shackle_telemetry module and add a section int he ready that explain how to enable it (e.g. setup the hook and add the telemetry dependency).

Comment on lines +118 to +120
shackle_telemetry:send(Client, iolist_size(Data)),
shackle_events:send(Client, iolist_size(Data)),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In either case, it is not a good idea to calculate the Data size here, until we know we will use it. It is better to use macro in general and pass Data to telemetry/events function to let it calc size only if needed (if dynamic handlers exist).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point!

I introduced a macro in shackle_events that will avoid computing these values and terms if there is no configured shackle_hooks handler.

@x0id
Copy link
Copy Markdown

x0id commented Aug 17, 2023

@rkallos It's much better now.
Still, I'm wondering if we could go further, to use macros like these:

process_responses([], _State) ->
    ok;     
process_responses([{ExtRequestId, Reply} | T], #state {
        client = Client,
        id = Id,
        queue = Queue
    } = State) ->

    ?EVT_REPLY(Client),
    case shackle_queue:remove(Queue, Id, ExtRequestId) of
        {ok, #cast {timestamp = Timestamp} = Cast, TimerRef} ->
            ?EVT_RESP_FOUND(Client, Timestamp),
            erlang:cancel_timer(TimerRef),
            reply(Reply, Cast, State);
        {error, not_found} ->
            ?EVT_RESP_NOT_FOUND(Client),
            ok
    end,
    process_responses(T, State).

and define them either for telemetry calls:

-define(EVT_REPLY(Client),
    telemetry:execute(
        [shackle, replies],
        #{count => 1},
        #{client => Client}
    )
).
    
-define(EVT_RESP_FOUND(Client, Timestamp), begin
    telemetry:execute(
        [shackle, found],
        #{count => 1},
        #{client => Client}
    ),  
    telemetry:execute(
        [shackle, reply],
        #{duration => timer:now_diff(os:timestamp(), Timestamp)},
        #{client => Client}
    )   
end).       
            
-define(EVT_RESP_NOT_FOUND(Client),
    telemetry:execute(
        [shackle, not_found],
        #{count => 1},
        #{client => Client}
    )
).

or for legacy shackle metrics:

-define(EVT_REPLY(Client),
    (fun
        ({Mod, Fun}) ->
            Mod:Fun(Client, counter, <<"replies">>);
        (_) ->
            ok
    end) (shackle_hooks:handler())
).  

-define(EVT_RESP_FOUND(Client, Timestamp),
    (fun
        ({Mod, Fun}) ->
            Mod:Fun(Client, counter, <<"found">>),
            Diff = timer:now_diff(os:timestamp(), Timestamp),
            Mod:Fun(Client, timing, <<"reply">>, Diff);
        (_) ->
            ok
    end) (shackle_hooks:handler())
).  

-define(EVT_RESP_NOT_FOUND(Client),
    (fun
        ({Mod, Fun}) ->
            Mod:Fun(Client, counter, <<"not_found">>, 1);
        (_) ->
            ok
    end) (shackle_hooks:handler())
).

The project can use some technique to define at compile time which hooks.hrl to use, allowing client's entirely custom solutions (for example, precompiled functions saved to persistent_term).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants